router redirect: use port from the request if redirect_port not specified #25573

cpakulski · 2023-02-15T16:12:41Z

Commit Message:
use port from the request if redirect_port not specified
Additional Description:
If redirect_port config item was empty, the redirect URL did not contain any port. Now the same port is used as in the original request.
Risk Level: Low
Testing: added unit tests and manual testing.
Docs Changes: yes
Release Notes: yes
Fixes #17318

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only · 2023-02-15T16:12:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #25573 was opened by cpakulski.

see: more, trace.

htuch

LGTM modulo comments, @yanavlasov to provide another set of eyes.

source/common/router/config_impl.cc

htuch · 2023-02-17T04:50:21Z

source/common/router/config_impl.cc

@@ -1117,6 +1109,17 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c
    final_port = redirect_config_->port_redirect_.c_str();
  } else {
    final_port = "";
+    const absl::string_view current_path = headers.getHostValue();


Out of curiosity, this is a direct response but not necessarily a redirect when you get here I think, what's to stop this applying even if it's not a redirect?

htuch · 2023-02-17T04:51:07Z

/lgtm api

howardjohn

I am worried this is a breaking change?

howardjohn · 2023-02-17T20:12:54Z

api/envoy/config/route/v3/route_components.proto

@@ -1659,6 +1659,7 @@ message RedirectAction {
      [(validate.rules).string = {well_known_regex: HTTP_HEADER_VALUE strict: false}];

  // The port value of the URL will be swapped with this value.
+  // If not specified, the request's port will be used.


The "request port" is ambiguous as there can be multiple ports associated with a given request. Is it from :authority? from the actual L4 port?

It is from headers, not L4.

howardjohn · 2023-02-17T20:14:18Z

changelogs/current.yaml

@@ -20,6 +20,9 @@ minor_behavior_changes:
 - area: http2
  change: |
    Request authorities are now validated with a library function from QUICHE rather than nghttp2. This behavior change can be reverted by setting ``envoy.reloadable_features.http2_validate_authority_with_quiche`` to ``false``.
+- area: router redirect
+  change: |
+    when ``port_redirect`` is not specified, the request's port is used.


Is this a breaking change? Our current API, which exposes this option, has the following enum option:

FROM_PROTOCOL_DEFAULT: automatically set to 80 for HTTP and 443 for HTTPS.

AFAIK this was the previous behavior in envoy with port unset. With this new change, it will use another port

AFAIK this was the previous behavior in envoy with port unset. With this new change, it will use another port

If port_redirect is set explicitly, there is no change in behavior. Does your API requires this param or can it be left unspecified?

Sorry to clarify, when a user sets that in our API we do not set port_redirect and explicitly expect the "automatically set to 80 for HTTP and 443 for HTTPS.". This behavior is changed by this PR, which I think represents a breaking API change for envoy.

Thanks for explaining. To be precise, Envoy did not add port 80 or 443, but simply stripped the incoming port. The result was that port was auto chosen based on the protocol: http or https.
Solution to that problem would be to set port_redirect to zero for reusing the port from the request. The logic would be:

port_redirect not specified - as before (strip port)

port_redirect = 0 (reuse from request)

port_redirect = value (use the value specified)

With a uint32 I don't think you can distinguish unset and 0?

yeah.. you are right :-(

API LGTM revoked until we resolve this, thanks for noticing @howardjohn

@howardjohn if Istio's API says

FROM_PROTOCOL_DEFAULT: automatically set to 80 for HTTP and 443 for HTTPS

It could now explicitly set these published port values in
https://github.com/istio/istio/blob/96f6a4f16d5a5aa4905ff0c0f94d9dd607b5f5f3/pilot/pkg/networking/core/v1alpha3/route/route.go#L603
rather than implictly assuming envoy will

from the original API PR istio/api#2088 it looks like this is what istio would have wanted envoy to do

It really doesn't matter what Istio does or docs say, this is a breaking change in Envoy which will extend beyond Istio.

phlax · 2023-02-21T08:16:53Z

/wait

github-actions · 2023-03-23T20:01:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cpakulski · 2023-03-24T14:47:33Z

I am planning to add a new config item with 3 options:

STRIP
AUTO (take port from request)
STATIC

It will be backwards compatible, so if not specified, it will behave as before (STRIP). If port_redirect is specified, it will take precedence over the new config item. port_redirect should be deprecated after (with deprecations guidelines: 12 months of support).

github-actions · 2023-04-23T16:01:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cpakulski · 2023-04-25T14:29:12Z

Closing for now. Will open a new PR with new API.

cpakulski added 3 commits February 14, 2023 21:20

redirect: preserve port if not specified in config

eb7cfd6

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into issue/17318

c6b1ce4

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

updated docs and release notes.

a5b7f5a

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only bot added the api label Feb 15, 2023

repokitteh-read-only bot assigned htuch Feb 15, 2023

htuch reviewed Feb 17, 2023

View reviewed changes

htuch assigned yanavlasov Feb 17, 2023

repokitteh-read-only bot removed the api label Feb 17, 2023

arkodg mentioned this pull request Feb 17, 2023

Clarify semantics when port_redirect is unset kubernetes-sigs/gateway-api#1740

Merged

howardjohn suggested changes Feb 17, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 21, 2023

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 23, 2023

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 24, 2023

This was referenced Apr 3, 2023

Redirect SchemeRewriteSpecifier doesn't seem to strip ports as specified #26516

Closed

Clarify port redirect behaviour and add conformance tests for the same kubernetes-sigs/gateway-api#1880

Merged

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 23, 2023

cpakulski closed this Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router redirect: use port from the request if redirect_port not specified #25573

router redirect: use port from the request if redirect_port not specified #25573

cpakulski commented Feb 15, 2023

repokitteh-read-only bot commented Feb 15, 2023

htuch left a comment

htuch Feb 17, 2023

htuch commented Feb 17, 2023

howardjohn left a comment

howardjohn Feb 17, 2023

cpakulski Feb 17, 2023

howardjohn Feb 17, 2023

cpakulski Feb 17, 2023

howardjohn Feb 17, 2023

cpakulski Feb 17, 2023

howardjohn Feb 17, 2023

cpakulski Feb 17, 2023

htuch Feb 17, 2023

arkodg Feb 17, 2023

howardjohn Feb 21, 2023

phlax commented Feb 21, 2023

github-actions bot commented Mar 23, 2023

cpakulski commented Mar 24, 2023

github-actions bot commented Apr 23, 2023

cpakulski commented Apr 25, 2023

router redirect: use port from the request if redirect_port not specified #25573

router redirect: use port from the request if redirect_port not specified #25573

Conversation

cpakulski commented Feb 15, 2023

repokitteh-read-only bot commented Feb 15, 2023

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Feb 17, 2023

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlax commented Feb 21, 2023

github-actions bot commented Mar 23, 2023

cpakulski commented Mar 24, 2023

github-actions bot commented Apr 23, 2023

cpakulski commented Apr 25, 2023